CSSTUDIO-3524 Fix equal spacing of widgets in Display Builder #3614
Conversation
…ts equally vertically.
…ts equally horizontally.
kasemir
left a comment
kasemir
left a comment
There was a problem hiding this comment.
use .. double instead of int
That brings up a pain point...
Until JavaFX, every graphics library known to mankind used integer coordinates, and that's perfect because the display is pixelated.
When we started the display builder, we naively kept that view of widget positions as integers.
JavaFX not only uses floating point, but also makes the whole numbers refer to the gridlines between pixels. The center of the top-left pixel is (0.5, 0.5), not (0,0).
See https://docs.oracle.com/javase/8/javafx/api/javafx/scene/Node.html, "Coordinate System".
When you draw a line from (1,1) to (10,10), you'll see that it's somewhat fuzzy because you're drawing the line along the gridline between pixels. Should for example have used (1.5, 1.5) to (9.5, 9.5). See also https://stackoverflow.com/questions/11886230/how-to-draw-a-crisp-opaque-hairline-in-javafx-2-2, https://stackoverflow.com/questions/11881834/what-are-a-lines-exact-dimensions-in-javafx-2, https://dlsc.com/2014/04/10/javafx-tip-2-sharp-drawing-with-canvas-api/
Bottom line, most of our drawing commands may be off by 0.5
|
@kasemir Thanks for the information, I was not aware of this. Going forward, this can be perhaps be taken into account, and gradually the drawing commands in Phoebus can be improved. |
...play/editor/src/main/java/org/csstudio/display/builder/editor/actions/ActionDescription.java
Outdated
Show resolved
Hide resolved
| int height = widget.propHeight().getValue(); | ||
|
|
||
| final int bottomCenter = location + height / 2; | ||
| final double bottomCenter = (double) location + (double) height / 2.0; |
There was a problem hiding this comment.
Cast to double should not be necessary.
There was a problem hiding this comment.
why not do
final double bottomCenter = (double) (location + height / 2.0);
the intent is clear
everything will be promoted to double
the code is more readable without a lot of casts
| final int topCenter = location + height / 2; | ||
| final int coffset = ( bottomCenter - topCenter ) / ( N - 1 ); | ||
| final double topCenter = (double) location + (double) height / 2.0; | ||
| final double coffset = ( bottomCenter - topCenter ) / ( (double) N - 1.0 ); |
There was a problem hiding this comment.
Cast to double should not be necessary.
| widget = sortedWidgets.get(i); | ||
| height = widget.propHeight().getValue(); | ||
| undo.execute(new SetWidgetPropertyAction<>(widget.propY(), ( topCenter + i * coffset ) - height / 2)); | ||
| undo.execute(new SetWidgetPropertyAction<>(widget.propY(), (int) Math.round(( topCenter + (double) i * coffset ) - (double) height / 2.0))); |
There was a problem hiding this comment.
Cast to double should not be necessary.
...play/editor/src/main/java/org/csstudio/display/builder/editor/actions/ActionDescription.java
Outdated
Show resolved
Hide resolved
| { | ||
| final List<Widget> widgets = editor.getWidgetSelectionHandler().getSelection(); | ||
| final UndoableActionManager undo = editor.getUndoableActionManager(); | ||
| final int N = widgets.size(); |
There was a problem hiding this comment.
As @georgweiss suggested you could just cast once here.
This pull request implements a bugfix to the functionality to space a set of widgets equally in either the horizontal or the vertical direction in Phoebus by changing the calculation of the new widget positions to use the type
doubleinstead ofint. This prevents rounding errors (caused by the use of integer arithmetic), which, when the set of widgets grows larger, compounds and can become noticeable. I have tested the change manually in the Display Builder.